Gallery block: convert gallery to a wrapper around the core image block #24039
Conversation
glendaviesnz
added
the
[Block] Gallery
|
Size Change: -3.02 kB (0%) Total Size: 1.17 MB
ℹ️ View Unchanged
|
ZebulanStanphill
added
[Type] Technical Prototype
| // The id value is stored in a data attribute, so when the | ||
| // block is parsed it's converted to a string. Converting | ||
| // to a string here ensures it's type is consistent. | ||
| id: toString( newImage.id ), | ||
| } ) ), | ||
| columns: columns ? Math.min( newImages.length, columns ) : columns, | ||
| } ); | ||
|
|
||
| replaceInnerBlocks( clientId, newBlocks ); |
There was a problem hiding this comment.
This may need to be replaced with getBlocksByClientId and insertBlocks to compare what is already in the gallery with what is passed back by media dialog if we want to preserve sort order changes between updates from the media browser
glendaviesnz
force-pushed
the
try/nested-image-gallery
branch
from
2671482
to
5fdbd06
Compare
glendaviesnz
changed the title
|
@mapk, @senadir, @talldan, @afercia, @ZebulanStanphill, @paaljoachim - what are your thoughts on this as a potential way forward with consistency between the image and gallery blocks. This seems to work reasonably well as a prototype, but no doubt there will be quite a bit of work getting the final pieces sorted, particularly deprecations, so would be good to get some idea of how feasible it is to get this into core before we commit to finishing it off. |
|
Hey Glen. This look really interesting. Inner child Image Blocks containing the Image Block sidebar settings and the parent Gallery Block with it's own sidebar settings. This seems to be the exact features I have mentioned in #11436 |
|
@glendaviesnz thanks for the ping. Conceptually, it makes sense to me that a Gallery is represented as a group of images. If that serves also the purpose to simplify things, then I'd totally second your proposal. 🙂 From an accessibility perspective, I see at least two big issues that need to be solved:
Keyboard In the current Gallery block, it is actually possible to tab through all the images and their controls because there's just one block. Semantics:
|
|
The list issue reminds me a lot of the discussion going on in #23915 right now with the Navigation block, where a similar issue has been encountered. Perhaps both may end up using the same solution, whatever that may be. |
|
Wow! ❤️ Thanks for this PR! It's been talked about for along time now. :) I couldn't get it working on my local env. When I switched to this branch, it would move all my content into a grid structure, not just the gallery. I'm not sure if that's something on my end though. I've got some quick feedback based on the gif above:
|
@ZebulanStanphill That's so true, and I think while we might have managed to avoid the problem for the nav block, given it's an issue here as well it seems a valid use case for blocks. I don't know that it's something that can be avoided for galleries.
@mapk I also had this issue, but I think it's still possible to test the gallery if it's the only block in a post. It seems to work pretty well from my testing. The tab behavior is an interesting one, because it's been designed this way to improve accessing the toolbar and sidebar. Arrow key nav seems to work in a grid like fashion as the table block does. |
|
Nice work! |
I'd say definitely not. The PR is already trying to disable that via block context. I haven't actually gotten around to testing the branch myself yet, though, so I don't know if it actually works or not.
Definitely agree.
Style variations could also be used to apply various CSS filters (e.g. grayscale, sepia, etc.), so I don't think there's any reason to disable style variations for the Image block in this context.
Again, we could use block context if we wanted to inherit this from the Gallery block. Perhaps we should still allow custom values per Image block, but have the Gallery block set the default. |
glendaviesnz
force-pushed
the
try/nested-image-gallery
branch
from
342c91a
to
6332ee5
Compare
@mapk - woops, the css selector nesting was a bit broken but didn't notice as I only had a single gallery block on my test post, have pushed a fix for this. The CSS for the grid is not perfect yet, some of the rows display the wrong height ... will look at moving the items to a list as @afercia suggests before I tidy this up, but you should at least be able to get it to display correctly with other blocks on the page now. |
Thanks for this feedback, I will see if the issues you outline can be resolved before we go any further. |
| @@ -87,13 +88,20 @@ export function ImageEdit( { | |||
| width, | |||
| height, | |||
| sizeSlug, | |||
| isInGallery, | |||
There was a problem hiding this comment.
will switch this to a context
There was a problem hiding this comment.
Perhaps the context name should be generalized to something that could be reused in other blocks like the Navigation block, e.g. isInList or isListItem.
There was a problem hiding this comment.
I added a context of isList as this seemed slightly more natural language wise, eg. if image context.isList then treat is as a list item ... but happy to revise if I am looking at it from the wrong angle.
Also, I till had to add an image attribute of isListItem still, but setting this based in context as I couldn't see any way to access the context in the save method, and we need to know there whether to wrap the image in an <li> or not as well as in edit ... let me know if I have missed a better way fo handling this.
There was a problem hiding this comment.
I'm not entirely sure which way to go with isList vs isListItem myself. It would be good to get some more opinions on this.
The edit markup should match the save markup as much as possible, so if it's being wrapped in a <li> in one, the other should match.
I would think (but could be wrong) that there should already be a way to access block context from the JS save component. But if not, the attribute is a decent workaround for now until that gets implemented.
There was a problem hiding this comment.
I'm now thinking we shouldn't be handling this via block context at all. See this comment. Of course, implementing a new InnerBlocks prop is outside the scope of this PR, so that task would have to be done in a separate PR.
There was a problem hiding this comment.
I would think (but could be wrong) that there should already be a way to access block context from the JS save
The docs seem to indicate that is is currently only accessible via JS in the edit method, and doing some debugging and looking through the code seems to confirm that, but let me if you come across a way of accessing context in the save method.
| <li className={ classes }> | ||
| <figure>{ figure }</figure> | ||
| </li> |
There was a problem hiding this comment.
On first glace, I would think that, similar to how the "block alignment" wrapping div works (see the code a few lines earlier), the classes should still be applied to the figure, rather than the li.
Additionally, I wonder what consequences this may have for styles targeting the wp-block-image class, since that could now apply to an li in some cases, rather than the figure. Either way, I suppose theme style issues are inevitable with this sort of massive change.
Looking at the code, it looks like alignment wrappers are specially handled by the BlockListBlock component to ensure the editor-only wp-block CSS class is applied to the alignment wrapper and not the wrapped contents.
Replicating the same behavior in this case would probably require a similar modification. But modifying that general component just for this one use-case would obviously be a hack.
However, if we ended up taking the hypothetical <InnerBlocks childWrapper="li" /> kind of approach instead of using block context, the block classes could stay on the figure with no problems.
Another point in favor of the InnerBlocks-prop approach: even if we generalize the block context name for this list-item wrapping, it's still left up to the individual blocks to implement the li wrapper, and it opens the door to inconsistent handling of where the block classes are put.
Additionally, the InnerBlocks-prop approach would allow this child-wrapper behavior to be generalized even further to cases where the wrapper could even have multiple levels, e.g. childWrapper={ MyWrapper } where MyWrapper is a component that returns something like <li><div>{ theChildBlock }</div></li>.
Another benefit would be that the child blocks would not need to be aware of the parent block to take advantage of this. The parent block would This would allow the Gallery block to more easily support 3rd-party image blocks without each of those blocks having to handle the li wrapping themselves.
Yet another benefit: with block context, something like isList would be passed down to not only direct children, but also children-of-children. Since the direct children may not themselves be lists, those children would have to override isList themselves to prevent their children from inheriting the same value. Unlike something like postId or postType, the child block wrapper is only relevant to the parent and its direct children. The InnerBlocks-prop approach doesn't have this problem.
Ultimately, the list-element wrapper feels more like a part of the parent, rather than the child. Though implementing an InnerBlocks childWrapper prop may result in a slight increase to the complexity of InnerBlocks, it would solve the issue in one spot and prevent other blocks from having to reimplement it over and over again, and it would remove the possibility for inconsistent handling of where the block classes get put.
I wasn't really sure before, but now I'm convinced that the InnerBlocks approach is the way to go.
There was a problem hiding this comment.
yeh, an InnerBlocks childwrapper prop would be a nice tidy decoupled way to handle this ... do you have any plans to explore adding this to InnerBlocks?
There was a problem hiding this comment.
It's completely untested and doesn't work for React Native yet, but I went ahead and threw together #24232 to implement that very feature.
Try rebasing your branch off of mine. You should be able to use the prop like so:
<InnerBlocks __experimentalItemWrapper="li" /><InnerBlocks.Content __experimentalItemWrapper="li" />Let me know if it works.
There was a problem hiding this comment.
sorry, got sidetracked with other project work today, will try and give it a go tomorrow
There was a problem hiding this comment.
That's okay; my branch had a bunch of obvious bugs until recently anyway. 😛
There was a problem hiding this comment.
Fixed this issue, but still getting the failing block validation ... will try and take a closer look at it this week.
There was a problem hiding this comment.
Got as far as working out that it is something to do with adding __experimentalItemCallback to InnerBlocks.Content in the save method.
Without this the block saves and validates fine on editor refresh, but of course then the nested image blocks our not wrapped in li in the frontend.
With the callback added the nested image blocks are correctly wrapped in <li>s on the front end, but block validation fails when reloading the editor.
There was a problem hiding this comment.
I looked into this, and I think the problem is not with your PR, but with the __experimentalItemCallback API; namely, I forgot to change any of the parsing code in #24232, so the editor doesn't know how to parse the post's comment-delimited HTML properly when the render callback API is used. See this comment. 😟
There was a problem hiding this comment.
whew, not just something simple/stupid that I was overlooking at least 😄
glendaviesnz
force-pushed
the
try/nested-image-gallery
branch
from
14fbf77
to
468aba4
Compare
Make InnerBlocks self closing in save method
glendaviesnz
force-pushed
the
try/nested-image-gallery
branch
from
468aba4
to
adaee2c
Compare
|
This will be very broken now - just rebased in order to get to a point to start work on this again, but will probably close this PR soon and start a fresh one. |
|
Sounds great, Glen! @glendaviesnz |
|
closing in favour of #25940 |
If it's on unordered list, why would order matter then? |
Description
This is a PoC to explore the feasibility of moving the Gallery block to be a wrapper around the Image block.
Related to #11436 & Automattic/jetpack#11794 (comment)
Initial work has not highlighted any major technical issues that would make this approach unfeasible.
Things left to implement:
How has this been tested?
Just manual tested currently during feedback phase.